Resolve .localhost to loopback per Fetch spec#4456
Resolve .localhost to loopback per Fetch spec#4456FelixVaughan wants to merge 4 commits intonodejs:mainfrom
Conversation
| } | ||
|
|
||
| // Implements the relevant part of "resolve an origin" for localhost public suffix | ||
| // https://fetch.spec.whatwg.org/#resolve-an-origin |
There was a problem hiding this comment.
This is fetch specific code and should not be in core
There was a problem hiding this comment.
I'll find a new home for it. Any suggestions?
| } | ||
|
|
||
| // Exact localhost or any subdomain of .localhost | ||
| return h === 'localhost' || h.endsWith('.localhost') |
There was a problem hiding this comment.
I am sure this can be more performant implemented
There was a problem hiding this comment.
I'll see what I can do
| const ipv4 = '127.0.0.1' | ||
| const ipv6 = '::1' | ||
|
|
||
| const results = [ |
There was a problem hiding this comment.
Why do you instantiate this array on every call?
There was a problem hiding this comment.
I could hoist out of the function but I think that would sacrifice readability for minor optimizations
| } | ||
| } | ||
|
|
||
| // Implements the relevant part of "resolve an origin" for localhost public suffix |
There was a problem hiding this comment.
Only the relevant part? What parts are messing?
There was a problem hiding this comment.
In the specs there are 4 different cases for origin resolution; only one has to do with localhost
If origin’s host’s public suffix is "localhost" or "localhost.", then return « ::1, 127.0.0.1 ».
Thats the one this addresses
|
|
||
| // Spec: If public suffix is localhost, resolve to loopback | ||
| if (isLocalhostPublicSuffix(hostname)) { | ||
| tlsOptions.lookup = localhostLookup |
There was a problem hiding this comment.
Have to check what this lookup attribute is doing
There was a problem hiding this comment.
Its purpose is to resolve a hostname. By default it performs a dns lookup but here we override it to short-circuit the lookup. Here's a link to the docs
KhafraDev
left a comment
KhafraDev
left a comment
There was a problem hiding this comment.
This isn't "per Fetch spec". The implementation is noticeably different.
@KhafraDev could point me in the right direction? |
|
There is nothing I can say here that wasn't already said in the issue. 3 possibilities:
This also has other issues like overriding tlsoptions.lookup which iirc can be set by users and following the fetch spec for some reason. You link this but none of the steps are actually implemented, and even if they were, undici hardly seems like the correct place to implement this. As mentioned in the issue, given the RFC, it seems like something that should be fixed in node core (it seems like |
|
Good to know I wasn't on the right path. I agree that ideally the main fix should be in core and I could open an issue there but I wanted to see if there's anything patchable here. I would be interested in hearing more about the Also not sure what |
3 participants
This relates to...
#4391
Rationale
Align localhost public-suffix resolution with the Fetch spec (Resolving domains -> “resolve an origin”): any
localhostor*.localhosthost resolves to loopback.Spec: Fetch 2.5 resolve an origin
Changes/Fixes
isLocalhostPublicSuffix(hostname)lookup(localhostLookup) for both net and TLS paths to short‑circuit DNS for.localhostlocalhosthosts or literal IPsStatus